Automated Test: feature-groups-authz-implementation #325
Conversation
…ation implementation Closes #35562 Signed-off-by: vramik <vramik@redhat.com>
📝 WalkthroughWalkthroughThis pull request introduces fine-grained group-based admin permissions by creating a GroupPermissionsV2 implementation that evaluates permissions using admin roles and authorization scopes. The changes refactor the permission system to use group IDs instead of GroupModel objects and update related permission evaluation classes to support new group management scopes (MANAGE_MEMBERSHIP, MANAGE_MEMBERS, VIEW_MEMBERS). Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant EP as Endpoint
participant GPv2 as GroupPermissionsV2
participant AuthZ as AuthorizationProvider
participant Res as Resource Store
participant Eval as Policy Evaluator
Admin->>EP: Request (check group permission)
EP->>GPv2: canManage(groupModel)
alt Has MANAGE_USERS Role
GPv2->>GP: hasOneAdminRole(MANAGE_USERS)
GP-->>GPv2: true
GPv2-->>EP: true
else No Admin Role
GPv2->>GPv2: hasPermission(groupId, MANAGE)
GPv2->>AuthZ: evaluate permission
AuthZ->>Res: get resource for group
Res-->>AuthZ: Resource
AuthZ->>Eval: check scopes (MANAGE)
Eval-->>AuthZ: ResourcePermission
AuthZ-->>GPv2: has scope
GPv2-->>EP: true/false
end
EP-->>Admin: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java (1)
73-98:⚠️ Potential issue | 🔴 CriticalFix listener registration to support both V1 and V2 feature flags.
The listener registration at line 74 only checks
ADMIN_FINE_GRAINED_AUTHZ(V1), but it should also register whenADMIN_FINE_GRAINED_AUTHZ_V2is enabled independently. The Profile feature definition shows V2 has no dependency on V1 and can be enabled alone (as demonstrated byExportImportTestWithFGAPV2Enabled). Without this fix, V2-only deployments will not register the permission cleanup listener, causing stale permissions to remain after role/client/group deletion.Change line 74 from:
if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) {to:
if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) || Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)) {server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
328-350:⚠️ Potential issue | 🟡 Minor
getResourceNamedoes not handleGROUPSresource type.The method has specific handling for
USERS(resolving the user ID to a username for display), but no equivalent forGROUPS. When this method is called for group resources, it falls through toresource.getDisplayName(), which returns the group ID instead of a human-readable group name. This affects the UI display in PolicyResourceService where these resource names are shown to users. Consider adding aGROUPScase similar to theUSERShandling to resolve and display the group name.
🤖 Fix all issues with AI agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java`:
- Around line 112-114: The current pipeline applies pagination inside
searchGroupsByAttributes then filters with groupsEvaluator::canView, which can
produce fewer than max results; update the logic in GroupsResource so permission
filtering occurs before pagination: retrieve the candidate stream/list (from
searchGroupsByAttributes or equivalent), apply groupsEvaluator::canView first,
then apply pagination (skip firstResult and limit to maxResults) and finally map
to GroupUtils.populateSubGroupCount/GroupUtils.toRepresentation; alternatively,
fetch a larger/unpaged candidate set and continue filtering until you produce
maxResults visible items, ensuring the methods GroupUtils.populateSubGroupCount
and GroupUtils.toRepresentation are applied only after permission filtering.
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java`:
- Around line 72-78: The Javadoc for canManage() is misleading because it
mentions AdminPermissionsSchema#VIEW (making it equivalent to canView()); update
the documentation on GroupPermissionEvaluator.canManage() to accurately reflect
its semantics—either remove the reference to AdminPermissionsSchema#VIEW and
state it returns true only for AdminRoles#MANAGE_USERS or
AdminPermissionsSchema#MANAGE (for V2), or explicitly document that it
intentionally differs from canView(); ensure you reference canView(),
canManage(), AdminRoles#MANAGE_USERS, and AdminPermissionsSchema#MANAGE in the
updated comment so readers can locate the related behavior.
- Around line 132-135: Update the Javadoc on requireManageMembers(GroupModel) to
reference the correct method canManageMembers(GroupModel) (instead of
canManageMembership), and ensure the description reads that it throws
ForbiddenException if canManageMembers(GroupModel) returns false; keep
references to ForbiddenException and GroupModel intact.
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java`:
- Around line 64-71: canManage() currently calls hasPermission(null,
AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE) which incorrectly
allows callers with only VIEW; update canManage() to only accept MANAGE by
removing AdminPermissionsSchema.VIEW from the hasPermission call so it matches
canManage(GroupModel) (keep the existing
root.hasOneAdminRole(AdminRoles.MANAGE_USERS) check and only pass
AdminPermissionsSchema.MANAGE to hasPermission).
- Around line 105-128: In getGroupIdsWithViewPermission(), replace uses of
groupResource.getId() with groupResource.getName() so hasPermission(...) checks
the resource name (matching resourceStore.findByName(server, groupId)) and the
returned Set contains group model IDs (what callers like
UsersResource.searchForUser() expect); update both the hasPermission(...) first
argument and the value added to granted, keeping the same
AdminPermissionsSchema.GROUPS_RESOURCE_TYPE lookup and semantics of
hasPermission/resourceStore.findByName/AdminPermissionsSchema.resolveGroup.
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java`:
- Around line 286-291: The test currently calls
realmAdminClient.realm(...).users().get(userAlice.getId()).joinGroup("no-such")
and asserts a ForbiddenException, which is fragile because a non-existent group
could yield NotFoundException; change the call to use a real group ID that the
admin is not permitted to manage (e.g., topGroup.getId()) so the authorization
check triggers reliably: replace the literal "no-such" with topGroup.getId() in
the joinGroup call while keeping the existing catch/assert block expecting
ForbiddenException.
- Line 68: Remove the stray extra semicolon after the field initializer for
topGroup in GroupResourceTypeEvaluationTest; locate the declaration "private
final GroupRepresentation topGroup = new GroupRepresentation();;" and change it
to a single semicolon at the end of the statement so it becomes "private final
GroupRepresentation topGroup = new GroupRepresentation();".
- Around line 163-166: The new group created via GroupRepresentation and saved
into testGroupId is not registered for cleanup immediately, so if the test fails
before re-parenting it under topGroup it will be leaked; after the creation call
that assigns testGroupId (the ApiUtil.handleCreatedResponse(...) call) register
the group for deletion/cleanup (using the test suite's cleanup helper) right
away—i.e., call the existing cleanup registration utility with testGroupId
before any further operations such as re-parenting to topGroup—to ensure the
group is removed even on intermediate test failures.
🧹 Nitpick comments (6)
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java (1)
142-147: Document the "empty set" contract more explicitly.The Javadoc says "If
canView()evaluates totrue, returns empty set" but doesn't clarify that empty means "no restriction / view all." Callers (likeBruteForceUsersResource) use!groupIds.isEmpty()to decide whether to apply group-scoped filtering. An explicit note like "an empty set indicates no group-level restriction is needed" would prevent misinterpretation.services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissionEvaluator.java (1)
32-164: Well-structured interface expansion with comprehensive Javadoc.The new method pairs (require/can) and their documentation clearly describe the permission model. Two minor observations:
- Line 120:
it's clientId→its clientId(possessive, not contraction).- Lines 163-164: The
@Deprecatedannotation onisImpersonatablelacks a replacement hint (e.g.,@deprecated Use {@link#canImpersonate(UserModel, ClientModel)} instead). This would help consumers migrate away from it.📝 Suggested Javadoc improvements
- * NOTE: If requester is provided, it's clientId is added to evaluation context. + * NOTE: If requester is provided, its clientId is added to evaluation context.+ /** + * `@deprecated` Use {`@link` `#canImpersonate`(UserModel, ClientModel)} instead. + */ `@Deprecated` boolean isImpersonatable(UserModel user, ClientModel requester);services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java (1)
130-165:hasPermissionevaluation logic is sound but lacks the resource ID guard present in UserPermissionsV2.In
UserPermissionsV2.hasPermission(line 123), the permission evaluation checkspermission.getResourceId().equals(resource.getId())before inspecting scopes. ThisGroupPermissionsV2.hasPermissiondoes not have that guard — it iterates all returned permissions regardless of their resource ID. If the policy engine ever returns permissions for other resources, this could lead to false positives.♻️ Add resource ID guard to match UserPermissionsV2 pattern
for (Permission permission : permissions) { - for (String scope : permission.getScopes()) { - if (expectedScopes.contains(scope)) { - return true; + if (permission.getResourceId().equals(resource.getId())) { + for (String scope : permission.getScopes()) { + if (expectedScopes.contains(scope)) { + return true; + } } } }services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java (1)
330-355: Redundantserver == nullchecks incanViewMembersandcanManageMembers.Both methods check
server == null(lines 338, 351-352) before callinghasPermission(group, ...), buthasPermissionalready performs the same null check internally (line 398-402). Not a bug, just unnecessary code.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java (2)
118-123: Consider usingassertThrowsinstead of try/catch +fail().This pattern is repeated throughout the file (lines 118, 151, 181, 206, 231, 241, 286). JUnit 5's
assertThrowsis more concise and idiomatic:assertThrows(ForbiddenException.class, () -> realmAdminClient.realm(realm.getName()).users().get(userAlice.getId()) .update(UserConfigBuilder.create().email("email@test.com").build()));
206-211: Nit: misleading representation passed toupdate().Line 207 passes
myGroupas the update body fortopGroup. While it doesn't matter because the call is expected to be denied, it's confusing to readers. Consider using a standaloneGroupRepresentationortopGroupto make the intent clearer.
|
|
||
| return stream.filter(groupsEvaluator::canView) | ||
| .map(g -> GroupUtils.populateSubGroupCount(g, GroupUtils.toRepresentation(groupsEvaluator, g, !briefRepresentation))); |
There was a problem hiding this comment.
Pagination before permission filtering may return fewer results than max.
The canView filter is applied after searchGroupsByAttributes has already applied pagination (firstResult, maxResults). If some groups are filtered out, the page will be smaller than maxResults. This is pre-existing behavior across all three search paths, but worth noting since the V2 per-group permission model makes it more likely that individual groups are filtered out (compared to a global admin role check that either allows all or none).
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java`
around lines 112 - 114, The current pipeline applies pagination inside
searchGroupsByAttributes then filters with groupsEvaluator::canView, which can
produce fewer than max results; update the logic in GroupsResource so permission
filtering occurs before pagination: retrieve the candidate stream/list (from
searchGroupsByAttributes or equivalent), apply groupsEvaluator::canView first,
then apply pagination (skip firstResult and limit to maxResults) and finally map
to GroupUtils.populateSubGroupCount/GroupUtils.toRepresentation; alternatively,
fetch a larger/unpaged candidate set and continue filtering until you produce
maxResults visible items, ensuring the methods GroupUtils.populateSubGroupCount
and GroupUtils.toRepresentation are applied only after permission filtering.
| /** | ||
| * Returns {@code true} if the caller has {@link AdminRoles#MANAGE_USERS} role. | ||
| * <p/> | ||
| * For V2 only: Also if it has permission to {@link AdminPermissionsSchema#VIEW} or | ||
| * {@link AdminPermissionsSchema#MANAGE} groups. | ||
| */ | ||
| boolean canManage(); |
There was a problem hiding this comment.
Javadoc for canManage() seems incorrect — includes VIEW scope.
The Javadoc states that canManage() returns true if the caller has a VIEW permission on groups. This would make canManage() and canView() equivalent in V2 mode. Either the Javadoc is wrong (should reference only MANAGE), or the semantics of the global canManage() are intentionally weaker than canManage(GroupModel). Please clarify.
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java`
around lines 72 - 78, The Javadoc for canManage() is misleading because it
mentions AdminPermissionsSchema#VIEW (making it equivalent to canView()); update
the documentation on GroupPermissionEvaluator.canManage() to accurately reflect
its semantics—either remove the reference to AdminPermissionsSchema#VIEW and
state it returns true only for AdminRoles#MANAGE_USERS or
AdminPermissionsSchema#MANAGE (for V2), or explicitly document that it
intentionally differs from canView(); ensure you reference canView(),
canManage(), AdminRoles#MANAGE_USERS, and AdminPermissionsSchema#MANAGE in the
updated comment so readers can locate the related behavior.
| /** | ||
| * Throws ForbiddenException if {@link #canManageMembership(GroupModel)} returns {@code false}. | ||
| */ | ||
| void requireManageMembers(GroupModel group); |
There was a problem hiding this comment.
Javadoc references wrong method: canManageMembership instead of canManageMembers.
requireManageMembers(GroupModel) should throw if canManageMembers(GroupModel) returns false, not canManageMembership(GroupModel) — these are two distinct methods with different semantics.
📝 Proposed fix
/**
- * Throws ForbiddenException if {`@link` `#canManageMembership`(GroupModel)} returns {`@code` false}.
+ * Throws ForbiddenException if {`@link` `#canManageMembers`(GroupModel)} returns {`@code` false}.
*/
void requireManageMembers(GroupModel group);🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java`
around lines 132 - 135, Update the Javadoc on requireManageMembers(GroupModel)
to reference the correct method canManageMembers(GroupModel) (instead of
canManageMembership), and ensure the description reads that it throws
ForbiddenException if canManageMembers(GroupModel) returns false; keep
references to ForbiddenException and GroupModel intact.
| @Override | ||
| public boolean canManage() { | ||
| if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) { | ||
| return true; | ||
| } | ||
|
|
||
| return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE); | ||
| } |
There was a problem hiding this comment.
canManage() incorrectly grants manage access to callers with only VIEW permission.
Line 70 passes AdminPermissionsSchema.VIEW as an accepted scope, meaning a caller with only VIEW permission on groups would pass the canManage() check. Compare with canManage(GroupModel) at line 79 which correctly checks only MANAGE. This appears to be a copy-paste from canView().
🐛 Proposed fix — remove VIEW scope from canManage()
`@Override`
public boolean canManage() {
if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) {
return true;
}
- return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE);
+ return hasPermission(null, AdminPermissionsSchema.MANAGE);
}🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java`
around lines 64 - 71, canManage() currently calls hasPermission(null,
AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE) which incorrectly
allows callers with only VIEW; update canManage() to only accept MANAGE by
removing AdminPermissionsSchema.VIEW from the hasPermission call so it matches
canManage(GroupModel) (keep the existing
root.hasOneAdminRole(AdminRoles.MANAGE_USERS) check and only pass
AdminPermissionsSchema.MANAGE to hasPermission).
| @Override | ||
| public Set<String> getGroupIdsWithViewPermission() { | ||
| if (root.users().canView()) return Collections.emptySet(); | ||
|
|
||
| if (!root.isAdminSameRealm()) { | ||
| return Collections.emptySet(); | ||
| } | ||
|
|
||
| ResourceServer server = root.realmResourceServer(); | ||
|
|
||
| if (server == null) { | ||
| return Collections.emptySet(); | ||
| } | ||
|
|
||
| Set<String> granted = new HashSet<>(); | ||
|
|
||
| resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> { | ||
| if (hasPermission(groupResource.getId(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) { | ||
| granted.add(groupResource.getId()); | ||
| } | ||
| }); | ||
|
|
||
| return granted; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how AdminPermissionsSchema creates/names group resources
echo "=== Searching for GROUPS_RESOURCE_TYPE usage ==="
rg -n "GROUPS_RESOURCE_TYPE" --type=java -C3
echo -e "\n=== Searching for resource creation patterns ==="
# Look for how resources are created with names
rg -n "resourceStore.create" --type=java -C2 | head -100Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 11494
🏁 Script executed:
#!/bin/bash
echo "=== Check GroupPermissionsV2.java structure ==="
cat -n services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java | head -150
echo -e "\n=== Check V1 GroupPermissions.java line 315 context ==="
sed -n '310,330p' services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java
echo -e "\n=== Check how individual group resources are created in GroupPermissionsV2 ==="
rg -n "resourceStore.create.*[Gg]roup" --type=java -C3Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 8081
🏁 Script executed:
#!/bin/bash
echo "=== Check for RESOURCE_NAME_PREFIX in GroupPermissions/V2 ==="
rg -n "RESOURCE_NAME_PREFIX" --type=java -C2
echo -e "\n=== Check GroupPermissions parent class to understand structure ==="
sed -n '1,100p' services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java
echo -e "\n=== Search for how group resources are registered/created in V2 context ==="
rg -n "getResourceTypeResource\|findByType.*GROUPS" --type=java -B3 -A3
echo -e "\n=== Check what the resource.getName() would return for GROUPS_RESOURCE_TYPE ==="
rg -n "getResourceTypeResource" --type=java -B5 -A10 | head -60Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 17957
🏁 Script executed:
#!/bin/bash
echo "=== Check AdminPermissionsSchema for resource creation details ==="
sed -n '95,130p' server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java
echo -e "\n=== Check what resources are created for group permissions in V2 initialization ==="
rg -n "GROUPS_RESOURCE_TYPE" --type=java -B5 -A15 | grep -A15 "AdminPermissionsSchema\|GroupPermissionsV2"
echo -e "\n=== Look for where group resources are actually created with group IDs in V2 ==="
rg -n "initialize|createOrUpdate" services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java
echo -e "\n=== Check MgmtPermissions to see if it creates individual group resources ==="
rg -n "GroupPermissions" services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.java | head -20Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 19586
🏁 Script executed:
#!/bin/bash
echo "=== Check resolveGroup to understand resource naming ==="
sed -n '130,150p' server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java
echo -e "\n=== Check if V2 creates individual group resources or only uses resource type ==="
rg -n "getOrCreateResource.*group\|createOrUpdate.*group" --type=java -i
echo -e "\n=== Look at the test to understand expected behavior ==="
sed -n '300,350p' tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java
echo -e "\n=== Check what resource.getName() returns ==="
rg -n "getResourceTypeResource" server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java -A5Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2364
🏁 Script executed:
#!/bin/bash
echo "=== Check the full AdminPermissionsSchema methods to understand resource/name mapping ==="
sed -n '145,175p' server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java
echo -e "\n=== Check resolveGroup method ==="
grep -n "resolveGroup" server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java -A10
echo -e "\n=== Check what createPermission does in test ==="
rg -n "private.*createPermission" tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java -A20
echo -e "\n=== Look for UsersResource to see how it uses the returned group IDs ==="
rg -n "getGroupIdsWithViewPermission" --type=java -B3 -A3Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 9847
🏁 Script executed:
#!/bin/bash
echo "=== Verify resource storage - how are resources created and retrieved ==="
rg -n "interface ResourceStore\|void create\|findByName\|findByType" server-spi-private/src/main/java/org/keycloak/authorization/store/ResourceStore.java | head -30
echo -e "\n=== Check Resource model to understand getId vs getName ==="
rg -n "interface Resource\|String getId\|String getName" server-spi-private/src/main/java/org/keycloak/authorization/model/Resource.java | head -20
echo -e "\n=== Verify the actual usage - how resources are created for groups in V2 ==="
rg -n "getOrCreateResource.*GROUPS\|AdminPermissionsSchema.SCHEMA" services/src/main/java/org/keycloak/services/resources/admin/permissions/ --type=java -B2 -A5Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 299
🏁 Script executed:
#!/bin/bash
echo "=== Find ResourceStore interface ==="
find . -name "ResourceStore.java" -type f 2>/dev/null
echo -e "\n=== Find Resource model interface ==="
find . -name "Resource.java" -path "*/authorization/model/*" -type f 2>/dev/null
echo -e "\n=== Check how group resources are created/identified ==="
rg -n "findByType.*GROUPS" --type=java -B5 -A10
echo -e "\n=== Check for any migration or initialization code ==="
rg -n "GROUPS_RESOURCE_TYPE" services/src/main/java/org/keycloak/services/resources/admin/permissions/ -B3 -A3Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 4848
🏁 Script executed:
#!/bin/bash
echo "=== Check Resource interface methods ==="
cat -n server-spi-private/src/main/java/org/keycloak/authorization/model/Resource.java | head -50
echo -e "\n=== Check how resources are created in AdminPermissionsSchema.getOrCreateResource ==="
sed -n '92,125p' server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java
echo -e "\n=== Trace the flow: what resource.getName() returns vs getId() ==="
echo "Looking for evidence of resource creation with group ID as name..."
rg -n "resourceStore.create.*name" server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java -B2 -A2Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 4156
🏁 Script executed:
#!/bin/bash
echo "=== Final confirmation: What does resolveGroup return and how it's used ==="
sed -n '181,186p' server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java
echo -e "\n=== Verify that resources ARE created with group ID as name in V2 flow ==="
echo "When getOrCreateResource is called with groupId, resolveGroup returns group.getId()"
echo "and that becomes the resource name in: resourceStore.create(resourceServer, name, ...)"
echo ""
echo "So resource.getName() should equal group.getId()"
echo "And resource.getId() is the internal UUID"
echo -e "\n=== Check how the test creates permissions - does it use group ID? ==="
rg -n "createPermission.*GROUPS_RESOURCE_TYPE" tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java -B3 -A10Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 1141
🏁 Script executed:
#!/bin/bash
echo "=== Confirm: in V2, are individual group resources created with group ID as the NAME? ==="
echo "Checking for any explicit resource creation for individual groups in V2..."
rg -n "getOrCreateResource\|initialize" services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java
echo -e "\n=== Check if V2 ever calls getOrCreateResource for individual groups ==="
rg -n "getOrCreateResource.*group\|getOrCreateResource.*Group" --type=java -i | head -20
echo -e "\n=== Verify: in hasPermission, what happens when we pass resource.getId()? ==="
echo "Line 141: Resource resource = groupId == null ? null : resourceStore.findByName(server, groupId);"
echo "This searches by NAME, but resource.getId() is the UUID"
echo "So the lookup will fail and fall back to checking 'all-groups' permission"
echo ""
echo "This is a BUG because:"
echo "1. It will never find the individual group resource (mismatched ID vs name)"
echo "2. It will return IDs instead of group model IDs"
echo -e "\n=== Final check: what type is the resource returned by findByType for GROUPS_RESOURCE_TYPE ==="
rg -n "findByType" server-spi-private/src/main/java/org/keycloak/authorization/store/ResourceStore.java -A5Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 1942
🏁 Script executed:
#!/bin/bash
echo "=== Confirm the complete flow for V2 resource creation and lookup ==="
echo ""
echo "1. When permissions are set up for a group:"
echo " - getOrCreateResource(session, server, 'Scopes', 'Groups', groupId) is called"
echo " - resolveGroup returns groupId (the group model ID)"
echo " - resourceStore.create(server, groupId, ...) creates resource with NAME=groupId"
echo ""
echo "2. In getGroupIdsWithViewPermission():"
echo " - findByType(..., 'Groups', ...) returns resources of type 'Groups'"
echo " - These ARE the individual group resources (one per group)"
echo " - groupResource.getName() = groupId (correct)"
echo " - groupResource.getId() = internal UUID (wrong)"
echo ""
echo "3. The bug:"
echo " - Line 122: hasPermission(groupResource.getId(), ...) passes UUID"
echo " - hasPermission line 141: resourceStore.findByName(server, groupId) searches by NAME"
echo " - Mismatch: searching for UUID by NAME field - will never match"
echo ""
echo "=== Verify resource type name matches ==="
rg -n "TYPE.*=.*\"Groups\"" --type=java | head -5Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 1128
getGroupIdsWithViewPermission() uses groupResource.getId() (resource internal UUID) instead of groupResource.getName() (group model ID).
Two critical issues:
-
Line 122:
hasPermission(groupResource.getId(), ...)passes the resource's internal UUID, buthasPermissionat line 141 callsresourceStore.findByName(server, groupId), which searches by resource name. Since resource names are set to the group model ID during creation (viaresolveGroupin AdminPermissionsSchema), searching for the UUID by name will never match, causing every lookup to fall through to the "all-groups" fallback check instead of finding the individual group permission. -
Line 123:
granted.add(groupResource.getId())adds resource internal UUIDs to the result set, but callers likeUsersResource.searchForUser()expect actual group model IDs, breaking permission filtering.
🐛 Proposed fix — use getName() instead of getId()
resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> {
- if (hasPermission(groupResource.getId(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) {
- granted.add(groupResource.getId());
+ if (hasPermission(groupResource.getName(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) {
+ granted.add(groupResource.getName());
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public Set<String> getGroupIdsWithViewPermission() { | |
| if (root.users().canView()) return Collections.emptySet(); | |
| if (!root.isAdminSameRealm()) { | |
| return Collections.emptySet(); | |
| } | |
| ResourceServer server = root.realmResourceServer(); | |
| if (server == null) { | |
| return Collections.emptySet(); | |
| } | |
| Set<String> granted = new HashSet<>(); | |
| resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> { | |
| if (hasPermission(groupResource.getId(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) { | |
| granted.add(groupResource.getId()); | |
| } | |
| }); | |
| return granted; | |
| } | |
| `@Override` | |
| public Set<String> getGroupIdsWithViewPermission() { | |
| if (root.users().canView()) return Collections.emptySet(); | |
| if (!root.isAdminSameRealm()) { | |
| return Collections.emptySet(); | |
| } | |
| ResourceServer server = root.realmResourceServer(); | |
| if (server == null) { | |
| return Collections.emptySet(); | |
| } | |
| Set<String> granted = new HashSet<>(); | |
| resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> { | |
| if (hasPermission(groupResource.getName(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) { | |
| granted.add(groupResource.getName()); | |
| } | |
| }); | |
| return granted; | |
| } |
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java`
around lines 105 - 128, In getGroupIdsWithViewPermission(), replace uses of
groupResource.getId() with groupResource.getName() so hasPermission(...) checks
the resource name (matching resourceStore.findByName(server, groupId)) and the
returned Set contains group model IDs (what callers like
UsersResource.searchForUser() expect); update both the hasPermission(...) first
argument and the value added to granted, keeping the same
AdminPermissionsSchema.GROUPS_RESOURCE_TYPE lookup and semantics of
hasPermission/resourceStore.findByName/AdminPermissionsSchema.resolveGroup.
| Keycloak realmAdminClient; | ||
|
|
||
| private final String groupName = "top_group"; | ||
| private final GroupRepresentation topGroup = new GroupRepresentation();; |
There was a problem hiding this comment.
Minor: double semicolon.
Extra semicolon at the end of the initializer.
- private final GroupRepresentation topGroup = new GroupRepresentation();;
+ private final GroupRepresentation topGroup = new GroupRepresentation();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private final GroupRepresentation topGroup = new GroupRepresentation();; | |
| private final GroupRepresentation topGroup = new GroupRepresentation(); |
🤖 Prompt for AI Agents
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java`
at line 68, Remove the stray extra semicolon after the field initializer for
topGroup in GroupResourceTypeEvaluationTest; locate the declaration "private
final GroupRepresentation topGroup = new GroupRepresentation();;" and change it
to a single semicolon at the end of the statement so it becomes "private final
GroupRepresentation topGroup = new GroupRepresentation();".
| GroupRepresentation group = new GroupRepresentation(); | ||
| group.setName("testGroup"); | ||
| String testGroupId = ApiUtil.handleCreatedResponse(realmAdminClient.realm(realm.getName()).groups().add(group)); | ||
| group.setId(testGroupId); |
There was a problem hiding this comment.
Missing cleanup registration for testGroupId.
If the test fails between creation (line 165) and re-parenting under topGroup (line 174), this group is leaked. Register cleanup immediately after creation so it's removed regardless of test outcome.
Proposed fix
String testGroupId = ApiUtil.handleCreatedResponse(realmAdminClient.realm(realm.getName()).groups().add(group));
group.setId(testGroupId);
+realm.cleanup().add(r -> r.groups().group(testGroupId).remove());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GroupRepresentation group = new GroupRepresentation(); | |
| group.setName("testGroup"); | |
| String testGroupId = ApiUtil.handleCreatedResponse(realmAdminClient.realm(realm.getName()).groups().add(group)); | |
| group.setId(testGroupId); | |
| GroupRepresentation group = new GroupRepresentation(); | |
| group.setName("testGroup"); | |
| String testGroupId = ApiUtil.handleCreatedResponse(realmAdminClient.realm(realm.getName()).groups().add(group)); | |
| group.setId(testGroupId); | |
| realm.cleanup().add(r -> r.groups().group(testGroupId).remove()); |
🤖 Prompt for AI Agents
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java`
around lines 163 - 166, The new group created via GroupRepresentation and saved
into testGroupId is not registered for cleanup immediately, so if the test fails
before re-parenting it under topGroup it will be leaked; after the creation call
that assigns testGroupId (the ApiUtil.handleCreatedResponse(...) call) register
the group for deletion/cleanup (using the test suite's cleanup helper) right
away—i.e., call the existing cleanup registration utility with testGroupId
before any further operations such as re-parenting to topGroup—to ensure the
group is removed even on intermediate test failures.
| try { | ||
| realmAdminClient.realm(realm.getName()).users().get(userAlice.getId()).joinGroup("no-such"); | ||
| fail("Expected Exception wasn't thrown."); | ||
| } catch (Exception ex) { | ||
| assertThat(ex, instanceOf(ForbiddenException.class)); | ||
| } |
There was a problem hiding this comment.
Fragile assertion: using a non-existent group ID expects ForbiddenException.
Line 287 passes "no-such" as a group ID and expects ForbiddenException. This implicitly asserts that the authorization check runs before group existence validation. If the server order changes, this would break with NotFoundException instead. Consider using topGroup.getId() (a valid group the admin lacks permission for) to make the test less coupled to server-side evaluation order.
🤖 Prompt for AI Agents
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java`
around lines 286 - 291, The test currently calls
realmAdminClient.realm(...).users().get(userAlice.getId()).joinGroup("no-such")
and asserts a ForbiddenException, which is fragile because a non-existent group
could yield NotFoundException; change the call to use a real group ID that the
admin is not permitted to manage (e.g., topGroup.getId()) so the authorization
check triggers reliably: replace the literal "no-such" with topGroup.getId() in
the joinGroup call while keeping the existing catch/assert block expecting
ForbiddenException.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes